Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate and fix migration #425

Merged
merged 6 commits into from
Nov 25, 2018
Merged

Consolidate and fix migration #425

merged 6 commits into from
Nov 25, 2018

Conversation

dartcafe
Copy link
Collaborator

@dartcafe dartcafe commented Nov 16, 2018

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works 👍

lib/Migration/Version009000Date20181116063745.php Outdated Show resolved Hide resolved
lib/Migration/Version009000Date20181116063745.php Outdated Show resolved Hide resolved
lib/Migration/Version009000Date20181116063745.php Outdated Show resolved Hide resolved
lib/Migration/Version009000Date20181116063745.php Outdated Show resolved Hide resolved
lib/Migration/Version009000Date20181116063745.php Outdated Show resolved Hide resolved
@nickvergessen
Copy link
Member

Also in theory your migrations are named wrongly.
See nextcloud/server#12275 (comment) for the intended schema.

So in theory your files should be called 0009

@dartcafe
Copy link
Collaborator Author

So in theory your files should be called 0009

OMG. You're right.

@dartcafe
Copy link
Collaborator Author

I can change this, but does this affect the migration system, if users installed the RCs?

@nickvergessen
Copy link
Member

Since RC is git only, you can do it.
Also your migrations look save enough to not cause troubles, because you check if the table exists before adding it again etc.

But you can simply change the file+class names and run the update locally and see if it does 💥

@dartcafe
Copy link
Collaborator Author

dartcafe commented Nov 25, 2018

@nickvergessen thanks for help

  • updated function names
  • removed unused code
  • consolidated migrations

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you never released this yet, consolidating is okay. But in theory you should not do this as soon as it was released. You should also never edit a released migration, just add a new one.

And you file names should be 0009 not 000009. But not too important.

But the && stuff needs changing

lib/Migration/Version000009Date20180202213017.php Outdated Show resolved Hide resolved
lib/Migration/Version000009Date20180202213017.php Outdated Show resolved Hide resolved
lib/Migration/Version000009Date20180202213017.php Outdated Show resolved Hide resolved
lib/Migration/Version000009Date20180202213017.php Outdated Show resolved Hide resolved
@dartcafe dartcafe changed the title Added default 0 to' vote_option_id' Consolidate and fix migration Nov 25, 2018
@dartcafe
Copy link
Collaborator Author

And you file names should be 0009 not 000009. But not too important.

@nickvergessen I am a nitpicker. :-) So thanks for the hint. I updated the file names.

@dartcafe
Copy link
Collaborator Author

Since you never released this yet, consolidating is okay. But in theory you should not do this as soon as it was released. You should also never edit a released migration, just add a new one.

@nickvergessen This was indeed because this was never released. I just wanted to make it more handy for the initial use of the migration system.

@dartcafe dartcafe dismissed nickvergessen’s stale review November 25, 2018 18:46

Removed th second table check.

@dartcafe dartcafe merged commit bf59ea3 into master Nov 25, 2018
@dartcafe dartcafe deleted the patch-migration branch November 25, 2018 18:47
@nickvergessen
Copy link
Member

👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants